Allow using service account token for Prometheus/Alertmanager auth (e…#1968
Allow using service account token for Prometheus/Alertmanager auth (e…#1968
Conversation
…ven not in openshift)
WalkthroughThis PR introduces support for using pod service account tokens for Prometheus/Alertmanager authentication. A new environment variable flag Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/robusta/integrations/openshift/token.py (1)
5-6: Consider updating the comment for clarity.The comment currently mentions "if openshift is enabled" but the token is now used for non-OpenShift clusters as well when
PROMETHEUS_CLUSTER_TOKEN_AUTHis set.Consider updating the comment to reflect the broader usage:
-# NOTE: This one will be mounted if openshift is enabled in values.yaml +# NOTE: This is the standard Kubernetes service account token path, used when IS_OPENSHIFT or PROMETHEUS_CLUSTER_TOKEN_AUTH is enabled TOKEN_LOCATION = '/var/run/secrets/kubernetes.io/serviceaccount/token'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/configuration/metric-providers-external.rst(1 hunks)docs/configuration/metric-providers-in-cluster.rst(1 hunks)src/robusta/core/model/env_vars.py(1 hunks)src/robusta/integrations/openshift/token.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.7)
src/robusta/integrations/openshift/token.py
6-6: Possible hardcoded password assigned to: "TOKEN_LOCATION"
(S105)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run_tests
- GitHub Check: run_tests
- GitHub Check: Deploy docs
🔇 Additional comments (5)
src/robusta/core/model/env_vars.py (1)
117-118: LGTM! Clean implementation of the new configuration flag.The new environment variable follows the established pattern and is appropriately placed alongside the related
IS_OPENSHIFTflag. The default value ofFalsemaintains backward compatibility.src/robusta/integrations/openshift/token.py (2)
3-3: Import looks good.The addition of
PROMETHEUS_CLUSTER_TOKEN_AUTHto the imports is clean and necessary for the updated logic.
10-11: Logic correctly extends token loading to non-OpenShift environments.The condition now properly checks both flags, allowing service account token authentication when either
IS_OPENSHIFTorPROMETHEUS_CLUSTER_TOKEN_AUTHis enabled. The parentheses ensure correct evaluation order.The implementation uses the standard Kubernetes service account token path (
/var/run/secrets/kubernetes.io/serviceaccount/token), which is mounted by default in all Kubernetes distributions, making this approach universally compatible. Theload_token()function is used for both AlertManager and Prometheus authentication contexts, with callers properly checking forNonereturn values.docs/configuration/metric-providers-in-cluster.rst (1)
95-103: Documentation is clear and well-placed.The new authentication option is properly documented with a clear example. The placement under the "Authentication" section is logical, and the YAML configuration example correctly shows the environment variable as a string value.
docs/configuration/metric-providers-external.rst (1)
58-67: Documentation is consistent and well-structured.The authentication documentation correctly mirrors the example in
metric-providers-in-cluster.rst, ensuring consistency across different Prometheus deployment scenarios. The placement after the Bearer Token example provides good context for users.
2 participants
…ven not in openshift)